refactor(admin): group the settings page into sections - #178
Merged
Conversation
The admin page was one long list of fields. It is now split into "Etherpad server", "Pad types and behaviour", "External pads and embedding" and "Diagnostics", with the pad type toggles at the top of their section and the field hints tied to their inputs. Saving and the diagnostic tools report next to their own buttons instead of into a single shared status line, so feedback appears where the action was triggered. The two areas update independently – a late response no longer clears the other one's result. Hints are linked with aria-describedby, the dynamic "no pad type enabled" hint announces itself via role="status", and spacing is set with explicit row classes rather than negative margins. Adds tests/js/admin-settings.test.js, the first coverage for src/admin-settings.js.
Code Review by Qodo
1.
|
PR Summary by QodoAdmin settings: sectioned layout + independent save/diagnostics status
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- The Playwright health-check helper still read the save status node, so the admin health check spec would have failed against the new markup. - Diagnostics fall back to the save status area when the diagnostics area is absent, instead of silently dropping the feedback. - The new unit tests mocked responses without `ok: true`, so they ran through the error path; they now assert the success path explicitly.
role="status" was set on an element toggled via `display`, so the text arrived as initial content rather than a change and screen readers commonly stayed silent. The element is now always rendered and the script writes or clears its text, which is what a live region reacts to; an empty hint collapses via `.ep-field-hint:empty`, so nothing moves visually. Re-rendering identical text is skipped so a server-rendered message is not announced on load. The six new hint ids gain the app's `epnc-` prefix, matching every other id in the template.
The explanations were written for the review and read as heavy in the final code. Kept only the reasoning that is not visible from the code itself, at roughly a third the length.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The admin page was one long list of fields. It is now split into four sections: Etherpad server, Pad types and behaviour, External pads and embedding and Diagnostics. The pad type toggles sit at the top of their section, and each hint sits directly under the field it belongs to.
Saving and the diagnostic tools now report next to their own buttons instead of into one shared status line. The two areas update independently – a late response no longer clears the other one’s result.
Accessibility: hints are linked with
aria-describedby, and the dynamic “no pad type enabled” hint announces itself viarole="status". Spacing comes from explicit row classes rather than negative margins.New:
tests/js/admin-settings.test.js– the first coverage forsrc/admin-settings.js, including a regression test for the independent status areas.Structural change only; no behaviour change to the settings themselves.